Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Dynatrace scaler #5685

Merged
merged 30 commits into from
Jul 30, 2024
Merged

feat: Dynatrace scaler #5685

merged 30 commits into from
Jul 30, 2024

Conversation

cyrilico
Copy link
Contributor

@cyrilico cyrilico commented Apr 11, 2024

Add new scaler for interacting with Dynatrace and its Get Metric Data Points API

Checklist

Closes #2411

Relates to kedacore/keda-docs#1360

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: damas <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: damas <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: damas <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: damas <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico cyrilico marked this pull request as ready for review April 24, 2024 14:40
@cyrilico cyrilico requested a review from a team as a code owner April 24, 2024 14:40
@zroubalik
Copy link
Member

zroubalik commented Apr 24, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

Please, rebase your branch @cyrilico , the e2e issue was related with some breaking changes in otel-collector which are already fixed in main branch (your tests hasn't been executed yet indeed)

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
cyrilico and others added 2 commits April 28, 2024 14:48
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico cyrilico requested a review from JorTurFer April 28, 2024 14:03
@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico
Copy link
Contributor Author

new changes, please rerun

@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@JorTurFer
Copy link
Member

Hello,
I've been testing this locally and I found some things:

  • The dynatrace operator fails because the token doesn't have enough permissions for it
  • The job generators aren't generating the load because the values are overridden
    image

For the dynatrace operator issue, I've created a new secret 'DYNATRACE_OPERATOR_TOKEN', please use it for the operator and the other one for metrics (including for reading them by KEDA)

After making these changes, I see that dynatrace operator has created more pods:
image

But the metric isn't still available:
image

I think that the issue can be in something related with the e2e test configuration because I can see that KEDA is querying the value and getting an empty response with status 200 (so from KEDA side seems that works)
image

@cyrilico
Copy link
Contributor Author

Appreciate the help @JorTurFer 🙏 What metrics are available in that page? The one I used is supposed to be a built-in, i.e., supported OOTB, so I'm curious as to what exactly is available in that dynatrace environment

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico
Copy link
Contributor Author

Any news @JorTurFer ? 🙏

@JorTurFer
Copy link
Member

Any news @JorTurFer ? 🙏

I have to apologize for not answering before, I've been involved on a huge working peak :(
I'll try to review this ASAP. I guess that the issue with the metrics was that they needed more time to be consolidated, maybe we can use an approach like the Azure Monitor one, just querying about any available metric as default and updating the ScaledObject to trigger the scaling. e.g: Imagine that you can return an arbitrary number and you use it for scaling, at the end of the day, we don't need to check that the vendor calculates correctly the metric but just the connectivity

@cyrilico
Copy link
Contributor Author

Thanks, I have noticed in the corporate instance I have access to that Dynatrace does take a minute (sometimes literally) to ingest a data point, so I believe that might be an interesting approach. I'll take a look at the Azure Monitor scaler and try to get some inspiration

@JorTurFer
Copy link
Member

For instance, influx does it too: https://github.com/kedacore/keda/blob/main/tests/scalers/influxdb/influxdb_test.go#L106-L117

It sets the expected value within the query and just updating the ScaledObject you change the received value.

And this is the Azure log analytics one (I said azure monitor worngly): https://github.com/kedacore/keda/blob/main/tests/scalers/azure/azure_log_analytics/azure_log_analytics_test.go#L113-L123

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico
Copy link
Contributor Author

@JorTurFer couldn't find an exact map to replicate that mechanism in Dynatrace (other than potentially mocking the API calls, which would invalidate the connection testing part), so I've tried a configurable default value filter available in their query language spec; can we have another e2e test run?

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

Hello,
I'm so sorry due to the long silent from my side and I'd like to apologize, my life's been complicated during last months. During this week and next week I'll be focus on this PR (and others) aiming to include them within next week release.
Thanks for your contribution :)

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

@cyrilico , I've updated the e2e test to use custom metrics. We have the full control over them and they don't need the agent or any other service, just POST to ingest and KEDA will read them 😄
It's the same approach that we use for Azure Application Insights

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress here

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the contribution! ❤️
PTAL @zroubalik

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great, there is one minor thing, do you think you can refactor the scaler metadata parsing (parseDynatraceMetadata())? We introduced a new simplified way of parsing, it would be great to include this change in this scaler. You can see details at #5797

Signed-off-by: cyrilico <19289022+cyrilico@users.noreply.github.com>
@cyrilico cyrilico requested a review from zroubalik July 30, 2024 10:29
@cyrilico
Copy link
Contributor Author

done @zroubalik 👌

@JorTurFer
Copy link
Member

JorTurFer commented Jul 30, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 12a529d into kedacore:main Jul 30, 2024
18 checks passed
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @cyrilico !

@satrox28
Copy link

satrox28 commented Aug 10, 2024

Thank you, makes life easier now. Don't have to deploy Prometheus server additionally on each cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide scaler for Dynatrace metrics
4 participants